feat(zerogit): auto-create a conventional branch before push/pr on default branch - #671

internal/zerogit/zerogit_test.go (2)

836-848: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

CurrentGitUser's fallback branches (OS username, literal "user") are untested.

Only the git config user.name success path is covered. The two fallback tiers (L715-718 in zerogit.go) have no coverage, at least a case where the fake runner errors/returns empty output to exercise the OS-username path.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/zerogit/zerogit_test.go` around lines 836 - 848, Extend
TestCurrentGitUser to cover CurrentGitUser’s fallback behavior when git config
returns an error or empty output, asserting the OS-username fallback and command
invocation; also add coverage for the final literal "user" fallback when no OS
username is available.

---

731-792: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Missing coverage for CreateBranch failing to check out (e.g. branch already exists).

All three subtests exercise success/dry-run/empty-name paths; none exercise the git checkout -b failure branch (L700-702 in zerogit.go), which is exactly the scenario that occurs on a name collision. Worth adding a subtest that returns a non-nil error/non-zero exit from the checkout call and asserts the wrapped error message.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/zerogit/zerogit_test.go` around lines 731 - 792, The
TestCreateBranch coverage should include checkout failure, such as an existing
branch name. Add a subtest alongside HappyPath and DryRunDoesNotCheckout using
fakeRunner to return a non-zero/error result for the checkout invocation, then
assert CreateBranch returns an error containing the wrapped checkout failure
message.

internal/cli/workflows.go (1)

1029-1060: 🚀 Performance & Scalability | 🔵 Trivial

Extra LLM round-trip added to every default-branch push/PR without --yes.

When on the default branch (and no --yes/dry-run), ensureFeatureBranch now performs its own StreamCompletion call to name the branch, on top of any existing commit-message-generation LLM call in the same flow. That's an additional ~60s-capped network round trip and provider cost on a very common path (any push straight from main). Worth being aware of for latency/cost budgeting, and consider whether the fallback slug is "good enough" to skip the LLM call by default (e.g., behind a flag) rather than always attempting it whenever a provider is configured.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/workflows.go` around lines 1029 - 1060, The ensureFeatureBranch
flow should not automatically perform an LLM request on every default-branch
push or PR. Use fallbackBranchSlug(summary) by default and gate
generateAutoBranchSlug behind an explicit opt-in configuration or flag,
preserving the existing branch-generation behavior when that opt-in is enabled.

internal/cli/workflow_test.go (1)

918-980: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

No CLI-level test for runChangesPR + ensureFeatureBranch integration.

Coverage is added for ensureFeatureBranch in isolation and for runChangesPush (Lines 918-980), but runChangesPR also now routes through ensureFeatureBranch and forwards the ensured branch into PushOptions.Branch (workflows.go Lines 924-928, 937). Given pr hardcodes dryRun=false unlike push, this path deserves its own targeted test (e.g. mirroring TestRunChangesPushCreatesFeatureBranchWhenOnDefault for changes pr on the default branch) to lock in the new behavior before it ships.

Want me to draft that test?

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/workflow_test.go` around lines 918 - 980, The CLI tests cover
feature-branch creation for runChangesPush but not the equivalent runChangesPR
flow. Add a targeted test for changes pr on the default branch, verifying
ensureFeatureBranch creates the expected branch and that runChangesPR forwards
it through PushOptions.Branch, while preserving the existing dryRun=false
behavior.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

internal/zerogit/zerogit_test.go (1)

527-668: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Missing coverage for Push's new fail-closed error path.

TestPushBranchesToRemote exercises isDefault == true (RejectsDefaultBranch) and isDefault == false (HappyPath, FlagsForceAndDryRun, FallbackRemoteToOrigin), but none of the subtests drive isDefaultBranch into returning an error (remote lookup fails and no local refs/remotes/<remote>/HEAD record) to verify Push's new wrapping at Line 585-587 ("cannot verify %q is not the default/protected branch: %w; use --yes to override"). This is the actual new logic added to Push in this PR and is otherwise only exercised indirectly via IsDefaultBranch's own FailsClosedWhenDefaultBranchUnknown test, which doesn't go through Push.

✅ Suggested additional subtest

t.Run("FailsWhenDefaultBranchCannotBeVerified", func(t *testing.T) {
    root := t.TempDir()
    runner := &fakeRunner{results: []CommandResult{
        {Stdout: root + "\n"},
        {Stdout: "feat/some-feature\n"},
        {Stdout: "origin\n"},
        {ExitCode: 128, Stderr: "fatal:"}, // ls-remote fails
        {ExitCode: 1},                     // no local refs/remotes/origin/HEAD record
    }}

    _, err := Push(context.Background(), PushOptions{
        Cwd:    root,
        RunGit: runner.Run,
    })
    if err == nil || !strings.Contains(err.Error(), "use --yes to override") {
        t.Fatalf("expected fail-closed error, got %v", err)
    }
})

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/zerogit/zerogit_test.go` around lines 527 - 668, Add a
FailsWhenDefaultBranchCannotBeVerified subtest to TestPushBranchesToRemote that
makes ls-remote fail and the local refs/remotes/origin/HEAD lookup fail, then
assert Push returns an error containing “use --yes to override.” Ensure the fake
runner sequence reaches Push’s wrapped isDefaultBranch error path rather than
the existing successful or protected-branch paths.

internal/cli/workflow_test.go (2)

824-824: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff

Four consecutive bool arguments are easy to transpose. ensureFeatureBranch(ctx, w, jsonMode, cwd, remote, allowDefaultBranch, dryRun, autoNaming, maxDiffBytes, deps) is called with four bare bools in a row at about fifteen new call sites. A swap between dryRun and autoNaming still compiles and silently exercises a different path. Consider a small options struct for the flags, or a test helper with named fields.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/workflow_test.go` at line 824, Update the ensureFeatureBranch
call sites in workflow tests, including the call in this test, to avoid
consecutive positional boolean arguments by introducing a named options struct
or test helper for jsonMode, allowDefaultBranch, dryRun, and autoNaming.
Preserve each test’s existing flag values while making the argument names
explicit.

---

2100-2120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Isolate this test from the developer's global git config. The test runs real git commit and git push with only user.name and user.email set locally. A contributor with commit.gpgsign = true, a custom core.hooksPath, init.templateDir, or a pre-commit hook in their global config gets a failing test that has nothing to do with the change. git init --bare -b main also requires git 2.28 or newer, so an older git fails instead of skipping.

Set GIT_CONFIG_GLOBAL and GIT_CONFIG_SYSTEM to a non-existent path in runWorkflowGit and in the CLI invocation, and disable signing explicitly.

🧪 Proposed hermetic setup

 func runWorkflowGit(t *testing.T, dir string, args ...string) string {
 	t.Helper()
 	cmd := exec.Command("git", args...)
 	cmd.Dir = dir
+	cmd.Env = append(os.Environ(),
+		"GIT_CONFIG_GLOBAL="+filepath.Join(t.TempDir(), "nonexistent-gitconfig"),
+		"GIT_CONFIG_SYSTEM="+filepath.Join(t.TempDir(), "nonexistent-gitsystem"),
+		"GIT_CONFIG_NOSYSTEM=1",
+	)
 	out, err := cmd.CombinedOutput()

 	runWorkflowGit(t, repo, "config", "user.email", "zero@example.invalid")
+	runWorkflowGit(t, repo, "config", "commit.gpgsign", "false")
+	runWorkflowGit(t, repo, "config", "core.hooksPath", filepath.Join(tmp, "no-hooks"))

Note that runWithDeps runs git through the production code path, so the same environment isolation must reach it for full hermeticity.

As per coding guidelines: "Code and tests must pass on Linux, macOS, and Windows."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/workflow_test.go` around lines 2100 - 2120, Make
TestRunChangesBareRemotePushThenPRUsable hermetic by configuring
GIT_CONFIG_GLOBAL and GIT_CONFIG_SYSTEM to nonexistent paths in runWorkflowGit
and the CLI invocation, including the runWithDeps production path. Explicitly
disable commit signing and skip the test when the installed Git does not support
git init --bare -b main, while preserving the existing test behavior.

Source: Coding guidelines

internal/cli/workflows.go (3)

527-540: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

The --message check is now duplicated. Lines 527-529 fully subsume the preceding options.message != "" check, and both emit the same error text. Remove the older check to keep one rule per flag.

♻️ Proposed cleanup

-	if command != "commit" && options.message != "" {
-		return options, false, execUsageError{"--message is only valid with `zero changes commit`"}
-	}
 	if command != "commit" && options.hasMessage {
 		return options, false, execUsageError{"--message is only valid with `zero changes commit`"}
 	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/workflows.go` around lines 527 - 540, Remove the redundant older
options.message != "" validation near the command-option checks, retaining the
existing `command != "commit" && options.hasMessage` rule and its error message
as the single --message validation. Leave the surrounding --auto and --dry-run
checks unchanged.

---

1344-1389: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

firstLine and plausibleLine can never differ. Both are assigned under !isPreambleText(candidate), and the extra clause (!isPreambleText(line) || candidate != line) on Line 1378 is implied by !isPreambleText(candidate): when candidate == line the two tests are identical, and when candidate != line the first clause is already true. plausibleLine is therefore always set whenever firstLine is set, so the return firstLine on Line 1388 is unreachable except for the empty case. Collapse them into one fallback variable.

Related edge case: isPreambleText treats any line ending in ., !, or ? as preamble, so a reply of add login page. yields an empty slug and an error from generateAutoBranchSlug. Consider trimming trailing sentence punctuation before the preamble test.

♻️ Proposed simplification

 func extractBranchSlug(text string) string {
-	var firstLine string
-	var plausibleLine string
+	var fallback string
 
 	for _, line := range strings.Split(text, "\n") {
@@
 		if slugLineRe.MatchString(candidate) && !isPreambleText(candidate) {
 			return candidate
 		}
 
-		if firstLine == "" && !isPreambleText(candidate) {
-			firstLine = candidate
-		}
-
-		if (!isPreambleText(line) || candidate != line) && !isPreambleText(candidate) {
-			if plausibleLine == "" {
-				plausibleLine = candidate
-			}
-		}
+		if fallback == "" && !isPreambleText(candidate) {
+			fallback = candidate
+		}
 	}
 
-	if plausibleLine != "" {
-		return plausibleLine
-	}
-	return firstLine
+	return fallback
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/workflows.go` around lines 1344 - 1389, In extractBranchSlug,
collapse firstLine and plausibleLine into a single fallback variable because
their current conditions are equivalent, and return that variable when no
slug-shaped candidate is found. Before applying isPreambleText, trim trailing
sentence punctuation from candidates so values such as “add login page.” remain
eligible for slug extraction and generateAutoBranchSlug does not receive an
empty result.

---

1265-1277: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Use path.Base for git paths. summary.Files[0].Path comes from git and always uses / separators. filepath.Base applies OS-specific separator rules, so the same input can split differently on Windows. Use path.Base for a platform-independent result.

♻️ Proposed change

-		return zerogit.SlugifyBranchComponent(filepath.Base(summary.Files[0].Path))
+		return zerogit.SlugifyBranchComponent(path.Base(summary.Files[0].Path))

As per coding guidelines: "Code and tests must pass on Linux, macOS, and Windows; canonicalize paths before comparison and avoid asserting raw temporary-directory spellings."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/workflows.go` around lines 1265 - 1277, Update
fallbackBranchSlug to use path.Base instead of filepath.Base when extracting the
filename from summary.Files[0].Path, preserving platform-independent handling of
git’s slash-separated paths.

Source: Coding guidelines

internal/zerogit/zerogit.go (2)

745-752: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the shared remote-resolution logic.

This block duplicates the remote resolution in Push at lines 581-588. The doc comment at lines 720-724 states the two must resolve remotes identically, so any future edit must touch both sites. Extract one helper, for example resolveRemoteForBranch(ctx, runGit, root, branch, options.Remote), and call it from both functions.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/zerogit/zerogit.go` around lines 745 - 752, Extract the shared
remote-resolution logic into a helper such as resolveRemoteForBranch, accepting
the context, Git runner, repository root, branch, and configured remote. Replace
the inline resolution blocks in both Push and the current function around the
branch remote lookup with calls to this helper, preserving the existing
configured-remote, branch-config, and origin fallback behavior.

---

998-1006: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Terminate option parsing in DeleteBranch for consistency.

Every other new helper in this file passes -- before remote or ref values. DeleteBranch passes fallbackBranch and branchToDelete directly. A branch name that starts with - would then be parsed as an option. Add -- to both commands.

🛡️ Proposed hardening

-	if _, err := gitOutput(ctx, runGit, cwd, "checkout", fallbackBranch); err != nil {
+	if _, err := gitOutput(ctx, runGit, cwd, "checkout", "--", fallbackBranch); err != nil {
 		return err
 	}
-	_, err := gitOutput(ctx, runGit, cwd, "branch", "-D", branchToDelete)
+	_, err := gitOutput(ctx, runGit, cwd, "branch", "-D", "--", branchToDelete)
 	return err

Note: git checkout -- <name> restores paths rather than switching branches. Use git switch -- <name> or validate the name instead; verify the chosen form before applying.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/zerogit/zerogit.go` around lines 998 - 1006, Update DeleteBranch to
terminate git option parsing for both fallbackBranch and branchToDelete. Use an
option-safe branch-switching command that preserves switching to the fallback
branch, then pass -- before both branch values in the switch and delete
commands; do not use checkout --, which changes the command’s meaning.

internal/cli/app.go (1)

92-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Name the parameters in these function types.

Fields such as commitsAhead func(context.Context, string, string, string) (int, error) and remoteHasBranch func(context.Context, string, string, string) (bool, error) give no hint of the cwd, remote, branch order. A caller that swaps remote and branch compiles cleanly and probes the wrong ref. Add parameter names to the type, for example func(ctx context.Context, cwd, remote, branch string) (bool, error).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/app.go` around lines 92 - 105, Update the function type
declarations in the surrounding struct, especially commitsAhead and
remoteHasBranch, to name each parameter and make the cwd, remote, and branch
ordering explicit. Apply descriptive names consistently to the other context and
string parameters without changing any signatures or behavior.

internal/zerogit/zerogit_test.go (1)

1537-1559: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Guard the branch.autoSetupMerge=inherit dependency.

If Git versions older than 2.35 are supported, skip this test or report the required Git version before using inherit. Git 2.35 introduced this value; older versions can reject it or fail to create origin/main, causing a misleading test failure.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/zerogit/zerogit_test.go` around lines 1537 - 1559, Update
TestHasUpstreamRejectsInheritedMainUpstream to detect the installed Git version
before configuring branch.autoSetupMerge=inherit, and skip the test with a clear
required-version message when Git is older than 2.35. Perform this check before
the inherit configuration or dependent checkout, while preserving the existing
test behavior for supported Git versions.

Source: Coding guidelines

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

internal/cli/workflows.go (5)

960-964: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Document why an unborn-probe error is ignored here.

Line 961 blocks only when unbornErr == nil && unborn. A probe failure lets the flow continue to push and PR creation. That is a deliberate open-fail on a preflight whose only job is a better error message, and the later push still fails closed. The surrounding comment does not say so. Add one sentence, because every neighboring check in this PR fails closed and a reader will assume this one does too.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/workflows.go` around lines 960 - 964, Add a concise comment
above the isUnbornRemote check explaining that probe errors are intentionally
ignored so the flow can continue, with the later push/PR operation providing the
fail-closed behavior; retain the existing condition and error handling
unchanged.

---

930-966: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

changes pr contacts the remote twice for the same answer.

Without --yes, Line 941 calls deps.isDefaultBranch only to learn the remote. ensureFeatureBranch then calls deps.isDefaultBranch again at Line 1111 with the same inputs. Each call runs git ls-remote --symref against the remote. The unborn probe adds a third ls-remote, and refreshTrackingRef adds a fetch. On a slow SSH remote the user waits for four sequential network round trips before anything is pushed.

Thread the first result into the preflight instead of repeating the lookup. One option: add an optional pre-resolved default-branch state parameter to ensureFeatureBranch, or move the unborn preflight inside it.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/workflows.go` around lines 930 - 966, Avoid resolving the
default branch twice in the changes pr workflow: reuse the result from the
initial deps.isDefaultBranch call when invoking ensureFeatureBranch, or move the
unborn-remote preflight into ensureFeatureBranch. Update ensureFeatureBranch and
its callers as needed so the existing remote and branch behavior remains
unchanged while eliminating the duplicate remote lookup.

---

1395-1409: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

plausibleLine and firstLine are always equal, so the second branch is dead.

At Line 1399 the guard reduces to !isPreambleText(candidate): if candidate == line, the second clause already implies !isPreambleText(line); if candidate != line, the first clause is true by construction. That is the same condition as Line 1395. Both variables therefore receive the same first value, and the plausibleLine return at Line 1406 can never differ from firstLine.

Collapse the two into one variable. TestExtractBranchSlug should keep passing unchanged, which confirms the redundancy.

♻️ Proposed change

-		if firstLine == "" && !isPreambleText(candidate) {
-			firstLine = candidate
-		}
-
-		if (!isPreambleText(line) || candidate != line) && !isPreambleText(candidate) {
-			if plausibleLine == "" {
-				plausibleLine = candidate
-			}
-		}
+		if firstLine == "" && !isPreambleText(candidate) {
+			firstLine = candidate
+		}
 	}
 
-	if plausibleLine != "" {
-		return plausibleLine
-	}
 	return firstLine

Remove the plausibleLine declaration at Line 1359 as well.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/workflows.go` around lines 1395 - 1409, In the branch-slug
extraction logic, remove the redundant plausibleLine variable and its assignment
condition, including its declaration. Keep a single firstLine value for the
first non-preamble candidate and return it directly, preserving the existing
behavior covered by TestExtractBranchSlug.

---

1106-1148: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Nil-guarding is inconsistent across the injected Git dependencies.

remoteHasBranch, refreshTrackingRef, isUnbornRemote, branchUpstreamRef, resetBranchRef, and deleteBranch are all nil-checked. inspectChanges, commitsAhead, headCommitSubject, currentGitUser, and createBranch are called directly. fillAppDeps populates the second group, so production is safe, but a unit test that omits one of them panics instead of failing with a message. State the invariant in the doc comment, or guard both groups the same way.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/workflows.go` around lines 1106 - 1148, Make dependency handling
consistent in ensureFeatureBranch and the related workflow: either document in
the relevant function comment that inspectChanges, commitsAhead,
headCommitSubject, currentGitUser, and createBranch are mandatory and must be
populated by fillAppDeps, or add nil guards matching remoteHasBranch,
refreshTrackingRef, isUnbornRemote, branchUpstreamRef, resetBranchRef, and
deleteBranch. Ensure omitted injected dependencies return a clear error instead
of panicking.

---

524-532: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Collapse the duplicated --message validation.

Line 524 and Line 527 return the identical error. options.message != "" is only reachable when options.hasMessage is true, so the first check is redundant. Keep the hasMessage form, which also catches --message "".

♻️ Proposed change

-	if command != "commit" && options.message != "" {
-		return options, false, execUsageError{"--message is only valid with `zero changes commit`"}
-	}
 	if command != "commit" && options.hasMessage {
 		return options, false, execUsageError{"--message is only valid with `zero changes commit`"}
 	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/workflows.go` around lines 524 - 532, In the validation block of
the workflow command parser, remove the redundant options.message != "" check
and retain the options.hasMessage condition to reject --message for non-commit
commands, including an explicitly empty value. Leave the subsequent commit
--message and --auto conflict validation unchanged.

internal/zerogit/zerogit.go (1)

873-884: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Harden option handling for branch and remote names.

• Do not add -- before the range in CommitsAhead; git rev-list treats following arguments as paths and exits with status 129. Use --end-of-options or a fully qualified remote-tracking ref.
• In DeleteBranch, use git switch -- <fallbackBranch>. git checkout -- <fallbackBranch> checks out a path, not a branch.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/zerogit/zerogit.go` around lines 873 - 884, Harden ref handling in
CommitsAhead and DeleteBranch: keep the rev-list range argument without --, but
prevent option interpretation by using --end-of-options or a fully qualified
remote-tracking ref; update DeleteBranch to switch to the fallback branch with
git switch -- <fallbackBranch> rather than checkout, preserving branch
semantics.

internal/cli/workflow_test.go (1)

2226-2245: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Isolate Git configuration for the entire test, not only runWorkflowGit.

Workflow Git commands also inherit the process environment, so global branch.autoSetupMerge can change branch creation and commit.gpgsign or core.hooksPath can break setup commits. Set the Git environment before setup and workflow execution. On Git 2.32+, use os.DevNull for GIT_CONFIG_GLOBAL and GIT_CONFIG_SYSTEM; otherwise use GIT_CONFIG_NOSYSTEM=1 with temporary global configuration paths. Skip Git versions older than 2.28 because git init --bare -b main is unsupported.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/workflow_test.go` around lines 2226 - 2245, Isolate Git
configuration for the entire TestRunChangesBareRemotePushThenPRUsable test,
including repository setup and workflow execution, by configuring the process
environment before any Git commands run. Use os.DevNull for GIT_CONFIG_GLOBAL
and GIT_CONFIG_SYSTEM on Git 2.32+, or GIT_CONFIG_NOSYSTEM=1 with temporary
global config paths on older supported versions; skip Git versions below 2.28
before using git init --bare -b main.

Source: Coding guidelines

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

internal/cli/workflows.go (1)

1091-1096: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Fix the truncated sentence in the doc comment.

Line 1091 begins "The working tree must be clean and HEAD must be ahead of the resolved remote" and never ends. Line 1092 starts a separate sentence with the function name. Two drafts appear to have been merged.

📝 Proposed fix

-// The working tree must be clean and HEAD must be ahead of the resolved remote
-// ensureFeatureBranch verifies working tree cleanliness, checks default branch
-// state, and auto-creates a feature branch if on the default branch. The
-// inspectChanges, commitsAhead, headCommitSubject, currentGitUser, and
-// createBranch fields on deps are mandatory dependencies populated by
-// fillAppDeps.
+// The working tree must be clean and HEAD must be ahead of the resolved
+// remote branch; both are verified before any branch is created. The
+// inspectChanges, commitsAhead, headCommitSubject, currentGitUser, and
+// createBranch fields on deps are mandatory dependencies populated by
+// fillAppDeps.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/workflows.go` around lines 1091 - 1096, Complete the opening
sentence of the doc comment before the “ensureFeatureBranch” sentence,
preserving the intended requirements that the working tree is clean and HEAD is
ahead of the resolved remote. Keep the existing dependency documentation
unchanged.

internal/zerogit/zerogit.go (1)

837-850: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Terminate option parsing on checkout -b and validate Name.

Every other new remote-facing call in this file uses -- and rejects option-shaped values. CreateBranch does not. options.Name is only trimmed. An exported caller that passes a name beginning with - makes Git parse it as an option. The current in-repo caller passes zerogit.BuildBranchName output, which is safe, so this is hardening at the API boundary rather than a live defect.

ResetBranchRef already validates its branch argument. Apply the same discipline here.

🛡️ Proposed hardening

 	name := strings.TrimSpace(options.Name)
 	if name == "" {
 		return BranchResult{}, fmt.Errorf("branch name required")
 	}
+	// refs/heads/<name> must stay inside the heads namespace, and the name
+	// must never reach `checkout` as an option.
+	if strings.HasPrefix(name, "-") || strings.HasPrefix(name, "/") ||
+		strings.Contains(name, "..") || strings.ContainsAny(name, "\\ \t\n") {
+		return BranchResult{}, fmt.Errorf("invalid branch name %q", name)
+	}

-	if _, err := gitOutput(ctx, runGit, root, "checkout", "-b", name); err != nil {
+	if _, err := gitOutput(ctx, runGit, root, "checkout", "-b", name, "--"); err != nil {
 		return BranchResult{}, fmt.Errorf("create branch %q: %w", name, err)
 	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/zerogit/zerogit.go` around lines 837 - 850, Harden CreateBranch by
validating options.Name with the same branch-name validation used by
ResetBranchRef, rejecting option-shaped or otherwise invalid names before Git
commands run. Update the checkout -b invocation to terminate option parsing with
-- while preserving the existing collision and branch-creation behavior.

internal/cli/app.go (1)

99-99: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Remove the unused branchHasUpstream dependency wiring.

appDeps.branchHasUpstream is only declared, wired in defaultAppDeps, and defaulted in fillAppDeps; it is never called. Remove the field and its wiring if the feature branch flow only needs remoteHasBranch; keep zerogit.HasUpstream and its tests.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/app.go` at line 99, Remove the unused appDeps.branchHasUpstream
field and its wiring from defaultAppDeps and fillAppDeps, provided the feature
branch flow relies only on remoteHasBranch. Preserve zerogit.HasUpstream and its
existing tests.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

internal/cli/workflow_test.go (2)

2325-2341: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Bind runWorkflowGit to the test deadline.

runGitCommand in internal/zerogit/zerogit_test.go derives a context from t.Deadline() before running git. runWorkflowGit uses a plain exec.Command. If a git subprocess hangs against the bare remote, this test blocks until the whole test binary times out, with no indication of which command stalled.

🧪 Proposed change

 func runWorkflowGit(t *testing.T, dir string, args ...string) string {
 	t.Helper()
-	cmd := exec.Command("git", args...)
+	ctx := context.Background()
+	if deadline, ok := t.Deadline(); ok {
+		var cancel context.CancelFunc
+		ctx, cancel = context.WithDeadline(ctx, deadline)
+		defer cancel()
+	}
+	cmd := exec.CommandContext(ctx, "git", args...)
 	cmd.Dir = dir

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/workflow_test.go` around lines 2325 - 2341, Update
runWorkflowGit to derive a context from t.Deadline() and run the git subprocess
with exec.CommandContext instead of exec.Command, preserving the existing
command arguments, working directory, and failure reporting.

---

828-828: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift

Consider an options struct for ensureFeatureBranch.

Every call site passes nine positional arguments, including four consecutive bools: jsonMode, allowDefaultBranch, dryRun, autoNaming. This file repeats that shape about twenty-five times. Transposing two of the bools compiles cleanly and silently changes which guard the test exercises, so a broken test can still pass for the wrong reason.

Grouping the flags into a small options struct would make each call site self-documenting and make the bool order impossible to get wrong.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/workflow_test.go` at line 828, Introduce a small options struct
for ensureFeatureBranch containing jsonMode, allowDefaultBranch, dryRun, and
autoNaming, then update ensureFeatureBranch and all call sites in this test file
to pass the struct instead of positional boolean arguments. Preserve existing
behavior and values while making each option explicitly named.

internal/cli/app.go (1)

640-641: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Make the restore opt-out explicit instead of relying on nil defaults.

fillAppDeps leaves deleteBranch and resetBranchRef nil so unit tests do not touch a real git tree. Production is correct today because Run passes defaultAppDeps(). The fragility is that ensureFeatureBranch silently skips the default-branch restore when resetBranchRef is nil. Any future caller that builds a partial appDeps loses a correctness behavior with no signal: local main keeps the pushed commits and diverges after a squash-merge.

Consider filling both from defaults like every other dependency and having tests inject explicit no-op stubs. That keeps the skip intentional and visible at the call site.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/cli/app.go` around lines 640 - 641, Update fillAppDeps to initialize
deleteBranch and resetBranchRef from defaultAppDeps rather than leaving them
nil, and adjust affected unit tests to inject explicit no-op stubs when real git
restoration or deletion should be skipped. Preserve ensureFeatureBranch’s
restore behavior for production and make test opt-outs explicit at their appDeps
construction sites.

internal/zerogit/zerogit_test.go (2)

763-789: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a case for the residual-upstream-mismatch branch.

SurfacesUpstreamWriteFailureWhenRecoveryFails covers the set-upstream-to command failing. The second guard in Push is not covered: set-upstream-to exits 0 but UpstreamRef still does not return <remote>/<branch>, which produces the "local upstream is still not" error. Add a fixture where the repair command succeeds and the follow-up UpstreamRef returns a different value.

🧪 Proposed additional subtest

t.Run("SurfacesUpstreamStillWrongAfterRepair", func(t *testing.T) {
	root := t.TempDir()
	runner := &fakeRunner{results: []CommandResult{
		{Stdout: root + "\n"},
		{Stdout: "user/slug\n"},
		{Stdout: "origin\n"},
		{Stdout: "ref: refs/heads/main\tHEAD\nabc123\tHEAD\n"},
		{Stdout: "To origin\n * [new branch] user/slug -> user/slug\n"},
		{ExitCode: 128, Stderr: "fatal: no upstream configured"},
		{Stdout: ""},              // branch --set-upstream-to succeeds
		{Stdout: "origin/main\n"}, // but the upstream is still wrong
	}}

	_, err := Push(context.Background(), PushOptions{Cwd: root, RunGit: runner.Run})
	if err == nil || !strings.Contains(err.Error(), "local upstream is still not") {
		t.Fatalf("expected residual-mismatch error, got %v", err)
	}
})

As per coding guidelines: "Every behavior or security-boundary change requires a regression test, including failure paths".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/zerogit/zerogit_test.go` around lines 763 - 789, Add a regression
subtest alongside SurfacesUpstreamWriteFailureWhenRecoveryFails that exercises
successful set-upstream-to recovery followed by UpstreamRef returning an
incorrect remote/branch. Configure fakeRunner with the full command sequence,
assert Push returns an error containing “local upstream is still not,” and
preserve the existing test’s coverage for repair-command failure.

Source: Coding guidelines

---

1461-1523: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider isolating ambient Git config in the initGitRepo-based tests too.

TestRemoteHasBranchSeesPushWithoutLocalUpstream and TestHasUpstreamRejectsInheritedMainUpstream pin GIT_CONFIG_GLOBAL, GIT_CONFIG_SYSTEM, and GIT_CONFIG_NOSYSTEM. The three tests here run the real git binary through initGitRepo without that isolation. The risk is lower because branch -M main normalizes the branch name and no push occurs, so this is not blocking. Setting the same three variables inside initGitRepo would make every real-git test in this file hermetic in one place.

As per coding guidelines: "Code and tests must pass on Linux, macOS, and Windows".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/zerogit/zerogit_test.go` around lines 1461 - 1523, Update
initGitRepo, or the shared setup it uses for real-git tests, to isolate ambient
Git configuration by setting GIT_CONFIG_GLOBAL and GIT_CONFIG_SYSTEM to
controlled test values and GIT_CONFIG_NOSYSTEM appropriately. Apply this
centrally so TestResetBranchRefMovesDefaultWithoutTouchingFeature,
TestResetBranchRefRefusesCheckedOutBranch, and
TestCurrentBranchReturnsCheckedOutName inherit the same hermetic setup, while
preserving cross-platform behavior.

Source: Coding guidelines

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

internal/zerogit/zerogit.go (1)

748-755: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the shared remote-resolution logic.

Push (lines 581-588) and IsDefaultBranch resolve the remote with identical code: explicit option, then branch.<name>.remote, then "origin". The CLI threads the remote from IsDefaultBranch into Push, so the two must stay identical. A single helper removes the risk of divergence.

♻️ Proposed helper

func resolveRemoteForBranch(ctx context.Context, runGit Runner, root, requested, branch string) string {
	if remote := strings.TrimSpace(requested); remote != "" {
		return remote
	}
	if upstream, err := gitOutput(ctx, runGit, root, "config", "branch."+branch+".remote"); err == nil && upstream != "" {
		return upstream
	}
	return "origin"
}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/zerogit/zerogit.go` around lines 748 - 755, Extract the duplicated
remote-selection logic into a shared resolveRemoteForBranch helper, preserving
the order of explicit trimmed remote, branch.<name>.remote configuration, and
"origin" fallback. Update both Push and IsDefaultBranch to use this helper so
their remote resolution remains identical.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes